Repository navigation
npu: derive the t2s stream from a t2 chunk, so the container is enough (P2) - #830
Conversation
The plan layer makes t2 the container form and the per-target stream a derivation of it, but `tools/q4nx_t2s.py` could only be fed from the pb16i image: a model shipped as t2 (6.82 GB) needed the 14.44 GB sidecar to reach the XDNA decode kernel. `from_t2(chunk, K, N)` closes that by unpacking the chunk to its codes and fp16 scale bits and re-deriving the image with the packer's own code, so the tile order and the int16 multipliers are the packer's and not a second implementation of them. t2 and t2s are the same weights in different orders, so they are never interchangeable; the check asserts both the byte equality against `from_pb16i_image()` and that the two orders actually differ, so a "simplification" that returns the chunk unchanged fails. `q4nx_t2.unpack` is split into `unpack_bits` (codes + fp16 bits) + `unpack` (the float32 view), because the conversion needs the stored bits rather than a rounded float; `unpack`'s arithmetic and signature are unchanged.
PR Reviewer Guide 🔍(Review updated until commit e84e3fe)Here are some key observations to aid the review process:
|
bong-water-water-bong
added
the
tprs-ready
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 7cc55aa0a — 4 files, +53/−9, and the new from_t2 is pinned equal to from_pb16i_image() by a gate
I ran.
python3 tests/q4nx_t2s_test.py -> 14 checks, 0 failure(s)
PASS stream size [139264 B] / ratio 0.47222x / 2.1250 bits per weight
PASS expand matches the pb16i code area byte for byte [262144 B, 0 differ]
PASS stream() == from_pb16i_image()
PASS t2 and t2s are different byte orders [135476 of 139264 bytes differ]
PASS from_t2(t2 chunk) == from_pb16i_image()
PASS a non-shared 128-K multiplier is refused
PASS reserved code refused by expand / by decode
The two-sided check is the right one: equality alone would pass a converter that returned the pb16i image back, and the
135,476-of-139,264 mismatch on the fixture is what proves from_t2 is not that. unpack_bits is the split the body
describes — stored fp16 bits out, float32 view unchanged — and the refactored q4nx_t2 path still passes its own gate.
What I re-derived myself: 23,674,880 = 10,880 tiles x 2,176 for gate/up at K=5120/N=17408, the same stream size #821
measured on the real image, and the 0.472222 ratio. What I did not run is the 431-image leg against the real
Bonsai-2 set (648 s under load); my attempt to spot-check one tensor died because the container on this box carries a
different chunk form for layers.0.mlp.gate_proj (55,705,600 B, not a 23,674,880 B t2 chunk), so that leg stays the
author's number. The synthetic gate pins the same equality against the packer's own code, which is why this is still
READY rather than held.
CI at this head: build pass 5m17s, CodeQL pass, pr_agent pass, hw_gate skipped. Behind main by #824/#825 only
(different files).
|
Persistent review updated to latest commit e84e3fe |
bong-water-water-bong
left a comment
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at e84e3fe32 — re-head only: own diff still 4 files / +53/-9, own files byte-identical to 7cc55aa0a,
and I re-ran the gate at this head: python3 tests/q4nx_t2s_test.py → 14 checks, 0 failure(s), including
from_t2(t2 chunk) == from_pb16i_image() and the two-sided "t2 and t2s are different byte orders [135476 of 139264
bytes differ]" that stops a converter passing by returning the input.
The one leg I did not run is unchanged and still the author's: the 431-image comparison (648 s). My attempt at a
one-tensor spot check failed for a reason worth knowing — the container on this box carries a different chunk form for
layers.0.mlp.gate_proj (55,705,600 B, not a 23,674,880 B t2 chunk) — so nothing about that leg changed.
CI at this head: build pass 5m29s, pr_agent pass (then re-run by the label), CodeQL + Analyze pass.
tprs-ready applied.
… instead of assigning The rebase onto #826/#830 left the forward assigning `i_qhalf_`/`i_kv_`, which P4 moved into `XdnaTarget` (`xdna_target.h:159` is their only declaration now) — so the pushed head did not compile. The setup path keeps the two design indices as locals and hands them to `xd_->set_role_split()`; every other use already asks the target by name (`role_is_pb16i_split`, `role_split_qhalf/kv`). No behaviour change: same indices, same order.
… instead of assigning The rebase onto #826/#830 left the forward assigning `i_qhalf_`/`i_kv_`, which P4 moved into `XdnaTarget` (`xdna_target.h:159` is their only declaration now) — so the pushed head did not compile. The setup path keeps the two design indices as locals and hands them to `xd_->set_role_split()`; every other use already asks the target by name (`role_is_pb16i_split`, `role_split_qhalf/kv`). No behaviour change: same indices, same order.
… instead of assigning The rebase onto #826/#830 left the forward assigning `i_qhalf_`/`i_kv_`, which P4 moved into `XdnaTarget` (`xdna_target.h:159` is their only declaration now) — so the pushed head did not compile. The setup path keeps the two design indices as locals and hands them to `xd_->set_role_split()`; every other use already asks the target by name (`role_is_pb16i_split`, `role_split_qhalf/kv`). No behaviour change: same indices, same order.
… instead of assigning The rebase onto #826/#830 left the forward assigning `i_qhalf_`/`i_kv_`, which P4 moved into `XdnaTarget` (`xdna_target.h:159` is their only declaration now) — so the pushed head did not compile. The setup path keeps the two design indices as locals and hands them to `xd_->set_role_split()`; every other use already asks the target by name (`role_is_pb16i_split`, `role_split_qhalf/kv`). No behaviour change: same indices, same order.
…pb16i's 410 (#1276) P3 (#802) landed as code (#826/#830/#837/#843) and is the container serving on this box, but its own gate was never recorded at the model level: the docs carry the phase win (0.589 vs 1.037 ms, 1.76x) and the leaderboard had no t2s row at all, so the claim 'halve the wire, speed up decode' was proven per kernel only. Adds the measured pair as leaderboard records - the t2s2 arm and the pb16i control it is read against, because a ratio is only a ratio if both arms are recorded - and regenerates docs/npu-leaderboard.md from them, plus a subsection in the t2s chunk of docs/npu.md that states what the stream is worth at the model level: pb16i (control) 410 ms/token (409-412), device wait 296, 12.93 GB/token t2s2 sealed 313 ms/token (311-317), device wait 209, 6.11 GB/token 1.31x per token, 1.42x on the device wait, on 2.12x fewer bytes (431 images, 14.44 -> 6.82 GB, checked on disk). The model keeps 1.31x of the kernel's 1.76x because the host half (66 ms) and the lm head (13.5 ms) do not shrink with the wire. Measured 2026-11-02 in three adjacent npu-hold tickets on :18099, amdxdna counter 0 in each, 7 steady-state tokens per arm from the serve's own [ph] tok line - not request walls, which pay the per-serve image fill once (614/651 for a 32-token request). Acceptance is byte-exactness: every decode role matched the pb16i path byte for byte at layer 0 (6/6) and layer 3 (8/8) on three seeds, and greedy text agrees on the shared prompt. Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg>
What
tools/q4nx_t2s.pycould only be fed from the pb16i image: a model shipped in the plan layer's owncontainer form (
t2, 6.82 GB for Bonsai-2 27B) could not be turned into the stream the XDNA decode kernelreads without the 14.44 GB sidecar.
from_t2(chunk, K, N)closes that gap.from_t2unpacks the chunk to its codes and fp16 scale bits, re-derives the image with the packer's owncode (
q4nx_pb16i.pack) and re-streams it. Going back through the packer is deliberate: the tile order andthe int16 multipliers are then the packer's, not a second implementation of them (the class of mistake that
cost two bugs in #809 — a hand-written transpose order).
tools/q4nx_t2.unpackis split intounpack_bits(codes int8[N][K]+ the stored fp16 bits[N][K//128])and
unpack(the float32 view), because the conversion needs the stored bits rather than a rounded float.unpack's signature and arithmetic are unchanged — re-measured, below.Gate, byte-exact
The check that matters is two-sided on purpose:
from_t2must equalfrom_pb16i_image(), and the t2chunk must differ from the stream (135,476 of 139,264 bytes on the fixture), because t2 is k-major within
each 32-row band and t2s is n-major within each 16x16 block. So a later "simplification" that returns the
chunk unchanged — the one wrong change that would look like a tidy-up — fails the test. Verified by breaking
it in place: a byte passthrough fails, and so does dropping the
.Ton the scales (both produce a stream thatis not the image's).
Not in this PR
npu-kernels(private), andnothing here touches the array.
ctest/build leg beyond CI's — the t2 chunk's C++ decoder (npu/q4nx.cpp) reads thecontainer, not the stream, and is untouched.
from_t2inherits pb16i's geometry on top of t2's, i.e.Nmust be a multiple of 1,024 as well as 32; a t2tensor outside that is refused by name (
is not a pb16i geometry) rather than silently reshaped. All 431Bonsai-2 images are whole images, so this costs nothing today.
Files
tools/q4nx_t2s.pyfrom_t2(), one import, one docstring paragraph (additive; the file is the owner's, from #821)tools/q4nx_t2.pyunpack_bits()extracted,unpack()becomes the float32 wrappertests/q4nx_t2s_test.pydocs/npu.mdpython3 tools/copyright.py --check→ rc 0.Context: the m16-ELF item that would let this lane claim a tokens/s number is still blocked (no AIE toolchain
on this box, kernels closed source), so this is the P2-format continuation instead — host-only, no window.